[v2] Make ToolTaskHandler.getTask/getTaskResult optional and actually invoke them#1764
[v2] Make ToolTaskHandler.getTask/getTaskResult optional and actually invoke them#1764felixweinberger wants to merge 4 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 86af2b6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
…actually invoke them These handlers were defined on the interface but never invoked — three code paths bypassed them and called TaskStore directly: - TaskManager.handleGetTask - TaskManager.handleGetTaskPayload - McpServer.handleAutomaticTaskPolling The handlers exist to support proxying external job systems (AWS Step Functions, CI/CD pipelines) where the external system is the source of truth for task state. But every test/example implementation was pure boilerplate delegation to the store. This change makes the handlers optional: when omitted, TaskStore handles requests (zero boilerplate, previous de-facto behavior). When provided, they're invoked for tasks/get, tasks/result, and automatic polling. Dispatch logic is isolated in ExperimentalMcpServerTasks. Core gains a single setTaskOverrides() method on TaskManager — no changes to public TaskManagerOptions type. McpServer itself gains ~5 lines. Also drops the Args parameter from TaskRequestHandler since tool input arguments aren't available at tasks/get/tasks/result time. BREAKING CHANGE: TaskRequestHandler signature changed from (args, ctx) to (ctx). ToolTaskHandler.getTask and getTaskResult are now optional. Closes #1332 Co-authored-by: Luca Chang <lucalc@amazon.com>
felixweinberger
force-pushed
the
fweinberger/tooltask-handlers
branch
from
b106062 to
0f7ba7c
Compare
felixweinberger
changed the title
TypeDoc can't resolve {@linkcode TaskStore} from the server package
since core isn't in the processed package set.
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR introduces an override mechanism into core TaskManager, has breaking changes to the experimental task API, and touches 13 files across core/server/tests — worth a human review for the design decisions involved.
Extended reasoning...
Overview
This PR makes getTask and getTaskResult optional on ToolTaskHandler and actually wires them into the tasks/get and tasks/result request paths, fixing a bug where these handlers were defined but never invoked. The changes span 13 files: core TaskManager gains a new TaskLookupOverrides type and setTaskOverrides() method; ExperimentalMcpServerTasks gains ~50 lines of dispatch logic (_taskToTool map, _recordTask, _installOverrides, _dispatch); McpServer is modified to eagerly construct the experimental module and install overrides at construction time; tests remove ~180 lines of boilerplate handlers; and migration docs are updated.
Security risks
No direct security risks. The override mechanism is internal (@internal annotation) and only invoked by McpServer. The _dispatch method validates that the tool exists and has the expected handler shape before calling it. No auth, crypto, or permissions code is affected.
Level of scrutiny
This warrants careful human review for several reasons:
- Core infrastructure change:
TaskManagerin@modelcontextprotocol/coregains a new internal method that alters the lookup flow fortasks/getandtasks/result— this affects all servers using tasks. - Breaking API changes:
TaskRequestHandlersignature changes from(args, ctx)to(ctx), andgetTask/getTaskResultbecome optional. Even though the API is experimental, downstream users may be affected. - Design decisions: The override pattern (McpServer installing overrides into core TaskManager at construction time) is an architectural choice that should be validated by a maintainer.
- Major version bumps: The changeset triggers Major bumps for
node,express, andhonopackages.
Other factors
The PR has good test coverage with two new tests (handlers invoked when provided, TaskStore consulted when omitted) and all existing tests updated. The in-memory _taskToTool map limitation is documented. The code is well-structured with clear separation of concerns. The net LOC change is negative (removing boilerplate), which is positive. However, the scope and the core changes justify human sign-off.
1 participant
Makes
getTaskandgetTaskResultoptional onToolTaskHandlerand wires them into thetasks/getandtasks/resultrequest paths. Supersedes #1332.Motivation and Context
These handlers were defined on the interface but never invoked — three code paths bypassed them and hit
TaskStoredirectly:TaskManager.handleGetTask→_requireTaskStore.getTaskTaskManager.handleGetTaskPayload→_requireTaskStore.getTask/getTaskResultMcpServer.handleAutomaticTaskPolling→ctx.task.storeThe handlers exist to support proxying external job systems (AWS Step Functions, CI/CD pipelines, etc.) where the external system is the source of truth for task state. But every test and example implementation was pure boilerplate delegation to
ctx.task.store, which made the handlers look vestigial.This PR makes them optional:
TaskStorehandles everything. Zero boilerplate.tasks/get,tasks/result, and automatic polling. Useful for external-system proxies.Isolation
All dispatch logic lives in
ExperimentalMcpServerTasks(the experimental module that already ownsregisterToolTask):_taskToToolmap,_recordTask(),_installOverrides(),_dispatch()— ~50 lines, all contained thereTaskManager.setTaskOverrides()+ two lookup checks. No changes to the publicTaskManagerOptionstype.How Has This Been Tested?
tasks/getandtasks/resultTaskStoreis consulted directly when handlers are omittedBreaking Changes
Yes (experimental API):
TaskRequestHandlersignature changed from(args, ctx)to(ctx)— tool arguments aren't available attasks/get/tasks/resulttimegetTaskandgetTaskResultare now optional onToolTaskHandlerSee
docs/migration.md.Types of changes
Checklist
Additional context
Known limitation: the taskId → tool mapping is in-memory and doesn't survive restarts or span multiple server instances. In those scenarios, requests fall through to
TaskStore. Documented on theToolTaskHandlerinterface.Also adds an
isToolTaskHandlertype guard replacing inline'createTask' in handlerchecks.Closes #1332